Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TypeScript definitions and test #119

Merged
merged 4 commits into from
Oct 3, 2018

Conversation

msssk
Copy link
Contributor

@msssk msssk commented Oct 1, 2018

This PR adds working TypeScript definitions (the DefinitelyTyped ones don't work). JavaScript projects can take two approaches to providing TS definitions:

The second approach is recommended by the TypeScript project. For projects that are willing to take on the maintenance of typedefs the first approach seems preferable to me. For projects that would rather focus strictly on JS and let the TS community handle typedefs, the second is clearly preferable.

@TehShrike indicated interest in maintaining typedefs within the deepmerge project, so I am submitting this PR.

  • index.d.ts: add typedefs
  • package.json
    • add typescript as dev dependency. Why >=2.2.2?
      • TS 2 is 2 years old
      • 2.0.2 is the first non-prelease V2 tag
      • npm install [email protected] results in 2.2.2
      • 3.1.1 is current; I have tested with 2.2.2 and 3.0.3
    • add script: test:typescript; also added it to the test script
    • the TS test invokes tsc.js directly since typescript/bin/tsc will not run on Windows
  • tests/typescript.ts: this file contains non-functional code that simply exercises the typedefs. If it compiles without error then the typedefs are not terribly broken (they could still be too loose, e.g. I don't think it's possible to specify a concrete type for the union of an arbitrary number of objects for merge.all, but I am not a TS master).

Copy link
Owner

@TehShrike TehShrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@TehShrike
Copy link
Owner

I'll merge this and deploy as a feature version bump later today.

package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -34,6 +35,7 @@
"rollup-plugin-commonjs": "8.2.1",
"rollup-plugin-node-resolve": "3.0.0",
"tap": "~7.1.2",
"typescript": ">=2.2.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a decently old version from some time in early 2017 from what I can see, is there a reason 3.1.1 was not chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numerous projects still use TS 2. All of the TS language features in the deepmerge typedefs were present in TS 2, so we may as well make it clear that TS 2 users can successfully use deepmerge with these typedefs. We're just setting a baseline here, saying "if you are using TS 2.2.2 or later then these typedefs should work for you."

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine with TS2-compatible typings. Until TypeScript adds recursive types, I don't think there's any reason to push forward.

@TehShrike TehShrike merged commit af971b1 into TehShrike:master Oct 3, 2018
@TehShrike
Copy link
Owner

Published as 2.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants